Fix on.needs being emitted into compiled workflow on: section - #49864
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49864 does not have the 'implementation' label and has 68 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
There was a problem hiding this comment.
Pull request overview
Fixes invalid workflow YAML caused by emitting compiler-only on.needs as a trigger.
Changes:
- Excludes
needswhen serializing theonsection. - Adds unit and integration regression coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/frontmatter_extraction_yaml.go |
Removes on.needs before YAML serialization. |
pkg/workflow/on_needs_integration_test.go |
Tests compiled output and dependency wiring. |
pkg/workflow/yaml_test.go |
Tests exclusion without input mutation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Check for needs: inside on: section (as a real key, not a job-level needs) | ||
| if inOnSection && strings.HasPrefix(trimmed, "needs:") && !strings.HasPrefix(trimmed, "#") { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Addressed in 06e7928. containsNeedsInOnSection now normalizes an optional leading # and fails if # needs: appears under the compiled on: block.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested. excludeMapKeys returns a new map (no mutation), the on.needs key is stripped only for the on: section, and downstream consumers (extractOnNeeds) are unaffected. Tests cover both the unit-level and integration-level scenarios including a raw-string check.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.7 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — one minor suggestion on the raw-text test helper, but the fix is correct and well-tested.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed:
excludeMapKeyscreates a clean copy before marshaling, so the original frontmatter map is not mutated — important for downstreamextractOnNeedsconsumers. - Test coverage good: Both a unit test (
TestExtractTopLevelYAMLSectionExcludesOnNeeds) and an integration test assertion cover the fix. - One fragility flag:
containsNeedsInOnSectionraw-text parser could false-positive on job-levelneeds:keys; the structured YAML assertion already provides sufficient coverage.
Positive Highlights
- ✅ No mutation of input map — defensive copy via
excludeMapKeys - ✅ Unit test explicitly verifies map immutability (the mutation guard)
- ✅ Clear PR description with before/after YAML examples
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.5 AIC · ⌖ 8.23 AIC · ⊞ 7.1K
Comment /matt to run again
| if trimmed == "on:" || strings.HasPrefix(trimmed, `"on":`) { | ||
| inOnSection = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
[/tdd] containsNeedsInOnSection parses raw YAML text to detect needs: inside on:. A needs: key at job level (e.g. jobs.activation.needs) could also match if the heuristic mis-tracks the section boundary — producing a false positive. The structured map check on line 55 already verifies the fix at the parsed-YAML level; the raw-text check adds fragility without extra safety.
💡 Suggested improvement
Either remove containsNeedsInOnSection (the structured assert on line 55 is sufficient), or tighten it so it cannot match needs: lines deeply indented inside jobs:. A comment explaining why both checks coexist would also help future readers.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 06e7928. I kept the raw-text regression check, but tightened it to only consider direct child keys within on: so it catches commented-out # needs: lines without matching unrelated nested/job-level needs: entries.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (9 tests)
Key Observations
Verdict
|
|
@copilot Please take a quick pass on the remaining PR hygiene items, then run the Notes to check before merging:
Branch refresh may help once you are ready.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Handled in 06e7928. I tightened the raw |
|
🎉 This pull request is included in a new release. Release: |
Specifying
on.needsin frontmatter caused theneeds:key to be serialized into the compiled workflow'son:section, producing invalid GitHub Actions YAML.Root cause
extractTopLevelYAMLSectionmarshaledfrontmatter["on"]directly to YAML without stripping theneedskey, which is a compiler-internal directive — not a valid GitHub Actionson:trigger.Changes
pkg/workflow/frontmatter_extraction_yaml.go— When serializing theon:section, callexcludeMapKeys(valueMap, "needs")to create a clean copy before marshaling. Theneedsdata is still consumed downstream byextractOnNeedsto wire job dependencies.pkg/workflow/on_needs_integration_test.go— Assert thatneedsis absent from the compiledon:section (both via parsed YAML and raw string check).pkg/workflow/yaml_test.go— AddTestExtractTopLevelYAMLSectionExcludesOnNeedsunit test verifying the key is excluded and the original map is not mutated.Before / after
The
on.needsjob-wiring behaviour forpre_activation/activation/agentis unchanged.Run: https://github.com/github/gh-aw/actions/runs/30771772506